Skip to content

Fix transport session id mismatch with sessionId#176

Open
CrazyHZM wants to merge 5 commits intomodelcontextprotocol:mainfrom
CrazyHZM:fix/session_id
Open

Fix transport session id mismatch with sessionId#176
CrazyHZM wants to merge 5 commits intomodelcontextprotocol:mainfrom
CrazyHZM:fix/session_id

Conversation

@CrazyHZM
Copy link
Contributor

@CrazyHZM CrazyHZM commented Apr 20, 2025 •
edited
Loading

Motivation and Context

Fix #168

How Has This Been Tested?

Breaking Changes

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@CrazyHZM
Fix transport session id mismatch with sessionId
b9e7d95 
Signed-off-by: JermaineHua <crazyhzm@apache.org>
@yu199195
Copy link

yu199195 commented Apr 21, 2025

good job，I also found this problem, now get the correct seesion, not inside the mcp session, but inside the transport～

chemicL
chemicL requested changes May 7, 2025
View reviewed changes
Copy link
Member

@chemicL chemicL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the proposal! I agree we have a mismatch and we need a way to provide the session id to both the transport and the session objects. I would propose that the McpServerSession.Factory expose a new method, generateId which can be fed into both of these objects. Would you like to apply such a change?

@tzolov tzolov added this to the 0.10.0 milestone May 7, 2025
@tzolov tzolov added the bug Something isn't working label May 7, 2025
@CrazyHZM
Copy link
Contributor Author

CrazyHZM commented May 9, 2025

Thank you for the proposal! I agree we have a mismatch and we need a way to provide the session id to both the transport and the session objects. I would propose that the McpServerSession.Factory expose a new method, generateId which can be fed into both of these objects. Would you like to apply such a change?

Yes, I'll make some revisions to this part.

CrazyHZM added 3 commits May 9, 2025 22:21
@CrazyHZM
Support generateId method for McpServerSession.Factory
499d114 
Signed-off-by: JermaineHua <crazyhzm@apache.org>
@CrazyHZM
Merge branch 'refs/heads/main' into fix/session_id
b00baaa 
# Conflicts:
#	mcp/src/main/java/io/modelcontextprotocol/server/McpAsyncServer.java
@CrazyHZM
Format the code style
f67abae 
Signed-off-by: JermaineHua <crazyhzm@apache.org>
@CrazyHZM CrazyHZM requested a review from chemicL May 9, 2025 14:27
@tzolov tzolov modified the milestones: 0.10.0, 0.11.0 May 12, 2025
chemicL
chemicL requested changes May 12, 2025
View reviewed changes
Copy link
Member

@chemicL chemicL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work on this. It is almost ready, a few nitpicks about imports.
As a next step we should unify the way the server transports log and include the session id everywhere. But please don't create a PR for it just yet.
@tzolov in 0.11.0 release notes we need to highlight that McpServerSession.Factory went through a breaking change impacting custom transport implementors, not end users per se.

@CrazyHZM
Remove unused import
a71e17f 
Signed-off-by: JermaineHua <crazyhzm@apache.org>
@CrazyHZM CrazyHZM requested a review from chemicL May 14, 2025 14:31
@schnaker85
Copy link

schnaker85 commented May 27, 2025

Any update on this one @chemicL? Is there still something missing for a merge?

@CrazyHZM
Copy link
Contributor Author

CrazyHZM commented Jun 13, 2025

ping @chemicL

@tzolov tzolov modified the milestones: 0.11.0, 0.12.0 Jul 30, 2025
@tzolov tzolov modified the milestones: 0.12.0, 0.13.0, 0.14.0 Sep 12, 2025
@tzolov tzolov modified the milestones: 0.14.0, 0.15.0 Oct 1, 2025
@tzolov tzolov modified the milestones: 0.15.0, 0.16.0 Oct 30, 2025
@tzolov tzolov modified the milestones: 0.16.0, 0.17.0 Nov 18, 2025
@tzolov tzolov removed this from the 0.17.0 milestone Dec 15, 2025
@tzolov tzolov added this to the 0.18.0 milestone Dec 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chemicL chemicL Awaiting requested review from chemicL

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

0.18.0

Development

Successfully merging this pull request may close these issues.

[Bug] mvc transport session id mismatch with sessionId

5 participants

@CrazyHZM @yu199195 @schnaker85 @chemicL @tzolov